Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Force loading of all aliases at startup #4

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

dave-newson
Copy link

This PR adds the ability to configure Class Alias Loader so that all class_alias maps are loaded on boot, rather than as a pre-loader to the standard auto-loader.

This behaviour can be activated by specifying:

    "extra": {
        "typo3/class-alias-loader": {
            "autoloader-mode": "force-alias-loading"
        }
    },

It merely cycles all the known class aliases (specified via Composer) and executes the class_alias call for each, providing a class name is not already loaded.

Risks

This autoloading method bypasses some of the safeties integrated in class-alias-loader:

  • All aliases are loaded at bootup. If you have a lot of aliases, this could badly affect startup performance.
  • Probably more likely that a bad alias, or dual-definition of a class will cause errors.
  • class-alias-loader doesn't run as an autoloader when this is enabled. Boo-hoo.

But Why

We're using Symfony and want to relocate some classes from A/B/C to D/E/F.
This can be achieved simply enough using class_alias, but by using class-alias-loader this makes integration a hundred times cleaner, and in prototype and development this worked a charm.

Unfortunately in production we also use ApcClassLoader, which wraps the Composer autoloader and caches the locations of Files which represent class names to be loaded.
This significantly speeds up production class autoloading when your project is as enormo-huge as ours.

ApcClassLoader needs to be at the top of the autoloading stack in order to intercept most (if not all) autoloader calls and load them from memory, rather than Composer.
ClassAliasLoader, which is loaded immediately after Composer's AutoLoader, also wants to be at the top of the stack.

Once symfony has booted, we end up with a structure like this:

ApcClassLoader
   ClassLoader
ClassAliasLoader
   ClassLoader

For whatever reason (I've not fully debugged it) the ApcClassLoader ended up obtaining a reference for the ClassAliasName => RealClassFileName, meaning that when it saw a reference to an Alias class name, it loaded the file for the REAL class name.

This results in the real class file being loaded, but as ClassAliasLoader is never called the alias is never established.

Other options

I tried putting the ClassAliasLoader in front of the ApcClassLoader, but the issue still occurred. It was also a performance problem to place that autoloader first when the ApcClassLoader should be prioritised in 99.999% of class loading cases.

I also tried letting the ApcClassLoader fail over to the ClassAliasLoader (as its next in the autoloader stack) but the ApcClassLoader was still ending up with the rogue class reference.

Besides this, we have some super bizzarro class referencing code that makes me wonder if it's even safe to lazy-load the class aliases on auto-load request.

Rather than poke these bears, I've chosen to just make class-alias-loader force-load all class_alias mappings at boot, and avoid heavy modifications to our complex applications class autoloader.

@dave-newson
Copy link
Author

I should point out I also modified how the Config class constants are defined, and how they get type-cast, because it annoyed me when adding new Config options. That's probably way out of scope and totally my personal preference.

Let me know if that's an unacceptable change.

@helhum
Copy link
Contributor

helhum commented Nov 13, 2016

First of all, let me thank you for trying out this package and creating such a great pull request.
It has all information I need to understand your case.

To the issue:

Your observation is correct an expected, that this autoloader has to be the only loader for classes that have an alias defined. If this is not the case, then you will run into all sorts of issues. Let me explain the why in more detail by going to each case that can happen.

1. An alias class name is requested by the application

Given we have a class name NewClass, which has exactly one legacy class name LegacyClass, which should be made an alias for compatibility reasons.

If the application uses LegacyClass the following must happen:

  1. The class file of NewClass needs to be loaded
  2. The alias needs to be established by executing class_alias('NewClass', 'LegacyClass')

That is the easiest and least problematic case.
Given we had a stack of two autoloaders ApcClassLoader on top, thus triggered first, and ClassAliasLoader as second. The ApcClassLoader isn't aware of the alias, thus ClassAliasLoader is triggered. It will first load the LegacyClass class file and the establish the alias.

Everything is fine for this case.

2. A class name, which has an alias, is requested by the application

Given we have a class name NewClass, which has exactly one legacy class name LegacyClass, which should be made an alias for compatibility reasons.

If the application uses NewClass the following must happen:

  1. The class file of NewClass needs to be loaded
  2. The alias needs to be established by executing class_alias('NewClass', 'LegacyClass')

These two steps need to be done be exactly one autoloader, otherwise you'll run into trouble. Here is why:

Given we had a stack of two autoloaders MyClassLoader on top, thus triggered first, and ClassAliasLoader as second.
Given MyClassLoader is able to find and load NewClass.

MyClassLoader is aware of NewClass and loads it without establishing LegacyClass as alias.

Everything is still working to some extend. If the application would now need LegacyClass, e.g. by creating an instance of it new LegacyClass(), PHP will trigger the autoloading again and we would go back to case one.

However, consider code like this:

interface A {
    public function(NewClass $newClass);
}

class B implements A {
    public function(LegacyClass $newClass) {}
}

If NewClass is already known by PHP, but LegacyClass not, because the the alias is not yet established, PHP will not trigger the autoloader again and just throws a fatal error.

Options

There are multiple options to fix this.

  1. Implement your own ApcClassLoader, which also stores the aliases into apcu and establish the aliases in case of a cache hit
  2. Use PHP 5.6 or higher and change ClassAliasLoader to store the alias maps in static properties of a generated class. This is the same optimization the composer class loader has undergone and will most likely make ApcClass loader obsolete, because this class will stay in OpCache memory after the first hit.
  3. If you really only have a few aliased classes and it does not harm your performance to load these classes and establish the aliases upfront, then just create a plain PHP file and set it as include un your composer configuration of the package in question. The you won't need this package any more

Conclusion

This alias loader package was designed to be able to deal with many classes and aliases. The option you introduced in the pull request, adds support for an edge case where only a few aliases are present. So it basically contradicts the initial intention of the package. This is the reason why I unfortunately cannot accept this PR as is.

Nevertheless I'm here to support you. I planned to implement option 2 anyway, so that we benefit from this speed gain ourselves. I will start implementing this now. By doing so, I will introduce API, that you can also go for option 1, which isn't easily possible now because of limitations in the API.

So I hope you won't choose option 3 and give this package another shot in the (near) future :)

@helhum helhum closed this Nov 13, 2016
@helhum
Copy link
Contributor

helhum commented Nov 13, 2016

If you don't mind, if I will consider your changes to the Config class in the refactoring I'm starting now. I'll try to cherry-pick this commit if possible.

@dave-newson
Copy link
Author

@helhum thanks for coming back with the detailed explanation. The autoloader system isn't something I'm intimately familiar with.

I fully agree with you about this being an edge-case for the ClassAliasLoader, and it definitely goes against the grain of the autoloader, but I don't think it's entirely out-of-bounds for this library as a feature.

If you really only have a few aliased classes and it does not harm your performance to load these classes and establish the aliases upfront, then just create a plain PHP file and set it as include un your composer configuration of the package in question. The you won't need this package any more

I'm fully aware that our application code is still growing, so eventually it won't be practical to use this alias force-loading mechanism, and we will have to migrate our system to use the class-alias-loader's real autoloader.
With that in mind, I'd much rather our developers wrote code that's compatible with the ClassAliasLoader rather than using raw class_alias calls from randomly included scripts dotted around our system.

I mean, I could make them specify the aliases in an array format that's compatible with class-alias-loader, and then i could write something that loads those alias mappings from a list of files that i make people specify somewhere (composer.json perhaps?) ... but I'm sure you can see where I'm going with this :)

This PR is option 3. It connects the dots between using class_alias by itself, and switching to the ClassAliasLoader autoloader. If someone was looking for a graceful upgrade path, this would be it.

I agree this feature is risky because of performance questions - you should absolutely know what you're doing before you opt into using the forced preloader. If you don't want to support that feature (and I can understand why) then I'm happy for you to discard this PR.

Unfortunately for us, it's too dangerous to modify our autoloader(s) at this time, so we'll be opting to use this branch and (hopefully) maintain compatibility with class-alias-loader until the issues can be solved (which will either be when performance issues become noticeable, ApcClassLoader becomes fully redundant, or ClassAliasLoader gets an update that happens to make it compatible).

Also, totally happy for you to cherry pick any of this PRs code, so no worries there.

@helhum
Copy link
Contributor

helhum commented Nov 13, 2016

OK. As said, I'm happy to help here and make this package more useful for other cases.
To do so: can you tell me what PHP version your application relies on? I'm currently considering to throw away legacy in the code base here, which was only needed for the time being and moving to a cleaner (and faster) approach. While doing so, I will definitely raise the minimum PHP requirement and ask myself if it makes sense to go for PHP 7 or if PHP 5.5 or 5.6 is better for users of this lib.

Do you have an opinion here?

Besides that, I reconsider merging your feature to the current branch and do the refactoring/rewrite in another one. This way, you could rely on a released version of this package (let's say 1.1.0), while I'm developing 2.0, where I definitely drop all the case insensitive class name handling and raise the minimum PHP requirement.

@helhum helhum reopened this Nov 13, 2016
@helhum
Copy link
Contributor

helhum commented Nov 13, 2016

Give me some time however to get things straight here …

@dave-newson
Copy link
Author

@helhum No worries, thanks for considering this further. We're happy to use branched code, but it is even better when we can use proper releases ;) No rush.

We're on PHP 5.6 at the moment, looking to upgrade to PHP7 in the next 6 months.
5.6 support would be ideal, although as this code needs to be as-fast-as-possible I could totally understand having two implementations - one for 5.6 (for BC/legacy) and another for 7+, if there's speed benefits to be had that aren't BC.

@helhum helhum force-pushed the master branch 2 times, most recently from 92b5928 to 994ee9b Compare March 29, 2020 16:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants